Skip to content

GH-145247: Use _PyTuple_FromPair[Steal] in Objects#145884

Merged
vstinner merged 16 commits intopython:mainfrom
sergey-miryanov:feat/145247-pytuple-from-pair-use-2
Mar 28, 2026
Merged

GH-145247: Use _PyTuple_FromPair[Steal] in Objects#145884
vstinner merged 16 commits intopython:mainfrom
sergey-miryanov:feat/145247-pytuple-from-pair-use-2

Conversation

@sergey-miryanov
Copy link
Copy Markdown
Contributor

@sergey-miryanov sergey-miryanov commented Mar 12, 2026 •
edited by bedevere-app bot
Loading

It is a second PR for usage of _PyTuple_FromPair[Steal] in the codebase.

@bedevere-app bedevere-app bot mentioned this pull request Mar 12, 2026
Open
methane
methane reviewed Mar 23, 2026
View reviewed changes
methane
methane reviewed Mar 23, 2026
View reviewed changes
methane
methane reviewed Mar 23, 2026
View reviewed changes
sergey-miryanov
sergey-miryanov commented Mar 23, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@sergey-miryanov sergey-miryanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Address review

vstinner
vstinner reviewed Mar 27, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduces a leak in the dict type (see my comment below about a leak).

@sergey-miryanov @vstinner
Apply suggestions from code review
5055af7 
Co-authored-by: Victor Stinner <vstinner@python.org>
@vstinner
Copy link
Copy Markdown
Member

vstinner commented Mar 27, 2026

Ah, there are now compiler warnings, about unused variables if I understood correctly:

Objects/odictobject.c expected 0 warnings, found 1
{'file': 'Objects/odictobject.c', 'line': '1159', 'column': '29', 'message': 'unused variable ‘item’', 'option': '-Wunused-variable'}

Objects/longobject.c expected 0 warnings, found 3
{'file': 'Objects/longobject.c', 'line': '4882', 'column': '15', 'message': 'unused variable ‘z’', 'option': '-Wunused-variable'}
{'file': 'Objects/longobject.c', 'line': '6113', 'column': '27', 'message': 'unused variable ‘result’', 'option': '-Wunused-variable'}
{'file': 'Objects/longobject.c', 'line': '6356', 'column': '15', 'message': 'unused variable ‘ratio_tuple’', 'option': '-Wunused-variable'}

@sergey-miryanov
Copy link
Copy Markdown
Contributor Author

sergey-miryanov commented Mar 27, 2026

Ah, there are now compiler warnings, about unused variables if I understood correctly:

Yeah, I fixed it. Working on left review comments.

@sergey-miryanov
Copy link
Copy Markdown
Contributor Author

sergey-miryanov commented Mar 27, 2026

@vstinner Thanks for review! Could you please take a look?

vstinner
vstinner reviewed Mar 27, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I have have a few more coding style suggestions.

This was referenced Mar 27, 2026
Merged
Merged
vstinner
vstinner reviewed Mar 27, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, more sugestions for dictitems_xor_lock_held() which is the most complex function of this change. I propose further changes to make the code easier to follow.

@vstinner vstinner enabled auto-merge (squash) March 27, 2026 21:31
@vstinner
Copy link
Copy Markdown
Member

vstinner commented Mar 27, 2026

Thanks for all updates! I enabled auto-merge on the PR. IMO this change makes the code easier to follow/review.

@sergey-miryanov
Copy link
Copy Markdown
Contributor Author

sergey-miryanov commented Mar 27, 2026

Thanks for review!

@vstinner vstinner merged commit a933e9c into python:main Mar 28, 2026
88 of 90 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@methane methane methane left review comments

@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra JelleZijlstra is a code owner

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sergey-miryanov @vstinner @methane